Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Козлов Данил #185

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

SenyaPevko
Copy link

ObjectPrinting/ObjectPrinting.csproj Outdated Show resolved Hide resolved
{
private readonly SerializationSettings _serializationSettings;

This comment was marked as resolved.


public PrintingConfig()
{
_serializationSettings = new SerializationSettings();

This comment was marked as resolved.


public HashSet<Type> GetFinalTypes()
{
return _finalTypes;

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

убрал доступ к публичным полям

typeof(DateTime), typeof(TimeSpan)
};

private readonly Dictionary<Type, CultureInfo> _cultureForType;

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

исправил

if (TrySerializeCollection(obj, nestingLevel, out var collectionSerialization))
return collectionSerialization;

if (settings.GetSerializedObjects().Contains(obj))

This comment was marked as resolved.

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

изменил отлов рекурсии. Добавил тест как на картинке

private Person _person;

[Test]
public void PrintToString_ShouldReturnStringWithEveryObjectProperty()

This comment was marked as resolved.


[Test]

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

добавил

return serializer.SerializeObject(obj, nestingLevel);
}

private string PrintEnumerableToString(IEnumerable<TOwner> enumerable)

This comment was marked as resolved.

public static class PropertyPrintingConfigExtension
{
public static PrintingConfig<TOwner> TrimmedToLength<TOwner>(
this PropertyPrintingConfig<TOwner, string> propConfig, int length)

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

сделал выбрасывание ошибки на такой случай

@gisinka
Copy link

gisinka commented Dec 25, 2023

Все комменты обсуждаемы - можем обсуждать или в тредах, или в личке в телеге

На пофикшенные комментарии можно ставить реакции или писать пометочки, что сделал и почему

if (TrySerializeFinalType(obj, out var typeSerialization))
return typeSerialization;

var identation = new string('\t', nestingLevel + 1);

This comment was marked as resolved.

{
if (!TrySerializeProperty(propertyInfo, obj, nestingLevel, out var propertySerialization))
continue;
objectSerialization.Append(identation + propertyInfo.Name + " = " + propertySerialization +
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Надо повыпиливать конкатенацию

private bool TrySerializeProperty(PropertyInfo propertyInfo, object obj, int nestingLevel,
out string serializedProperty)
{
serializedProperty = "";

This comment was marked as resolved.

if (settings.GetFinalTypes().Contains(type))
{
serializedType = obj.ToString();
if (settings.GetTypesWithCulture().ContainsKey(obj.GetType()) && obj is IFormattable)

This comment was marked as resolved.

var identation = new string('\t', nestingLevel + 1);
var objectSerialization = new StringBuilder();
objectSerialization.AppendLine(type.Name);
foreach (var propertyInfo in type.GetProperties())

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

добавил сериализацию полей

return new PropertyPrintingConfig<TOwner, TPropType>(this, propertyInfo);
}

public PrintingConfig<TOwner> PrintRecursion(Exception exception)

This comment was marked as resolved.


namespace ObjectPrinting
{
public static class MemberInfoExtension
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internal, это ведь твоя внутрянка


public PropertyPrintingConfig<TOwner, TPropType> Using(Func<dynamic, string> printProperty)
{
propertiesSerialization.TryAdd(propertyInfo, printProperty);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не совсем понял, почему именно Try, может затирать?

@@ -5,8 +5,11 @@ namespace ObjectPrinting.Tests
public class Person
{
public Guid Id { get; set; }

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Можно и без пустых строк

}

private string PrintToString(object obj, int nestingLevel)
private string SerializeObject(object obj, IImmutableList<object> previousObjects)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Немного пересложнил, здесь явно нужна некоторая декомпозиция, мб вынос в отдельную абстракцию сериалайзера

{
var firstStudent = new Student { Name = "Alex", Age = 10 };
var secondStudent = new Student { Name = "Miha", Age = 15 };
var thirdstudent = new Student { Name = "Petr", Age = 12 };
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Нужно немного поработать с неймингом, периодически переменные в тестах имеют длинные названия, что понижает читаемость, иногда теряется camelCase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants